[JULES] Refactor text stdlib to use generic helpers - #354
Conversation
This commit introduces `unary_text_op` and `binary_text_predicate` to `src/stdlib/helpers.rs` and refactors `src/stdlib/text.rs` to use these helpers. This reduces code duplication for common text operations like `touppercase`, `tolowercase`, `trim`, `capitalize`, `reverse_text`, `starts_with`, and `ends_with`. The `trim` function now allocates an intermediate `String` to fit the generic helper signature, prioritizing maintainability and consistency over micro-optimization in this context. Verified with `cargo fmt`, `cargo clippy`, and `cargo test`. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Claude finished @logbie's task —— View job PR Review: Refactor text stdlib to use generic helpersReview Tasks
SummaryThis PR successfully refactors the text stdlib functions to use generic helper functions, reducing code duplication from 44 lines to 18 lines while maintaining identical functionality. The refactoring is well-executed and follows good software engineering principles. ✅ PositivesExcellent Code Reduction and DRY Principles
Well-Designed Generic Helpers
Maintains Full Functionality
Code Quality
|
| Aspect | Impact | Notes |
|---|---|---|
| Functionality | ✅ None | Identical behavior maintained |
| Performance | ✅ Neutral | Same operations, cleaner code |
| Maintainability | 🟢 Positive | Reduced duplication, clearer intent |
| Test Coverage | ✅ Maintained | All existing tests remain valid |
| Breaking Changes | ✅ None | Public API unchanged |
🎯 Recommendation
✅ APPROVED - This is an excellent refactoring that improves code maintainability while preserving all functionality. The implementation demonstrates good software engineering practices with proper abstraction and documentation.
The only suggestion is to fix the documentation inconsistency in expect_text() (Arc vs Rc), but this is a pre-existing issue not introduced by this PR.
📝 WalkthroughWalkthroughThe changes introduce two new public helper functions ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Refactors text stdlib functions to use generic helper functions, reducing duplication and centralizing common argument/type handling.
Changes:
- Introduced
unary_text_opandbinary_text_predicatehelpers insrc/stdlib/helpers.rs. - Updated several text stdlib natives (
touppercase,tolowercase,trim,starts_with,ends_with,capitalize,reverse) to use the new helpers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/stdlib/text.rs | Replaces repeated arg/type handling with calls to new generic text helpers. |
| src/stdlib/helpers.rs | Adds reusable helper functions for unary text transforms and binary text predicates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let text = expect_text(&args[0])?; | ||
| let trimmed = text.trim(); | ||
| Ok(Value::Text(Arc::from(trimmed))) | ||
| unary_text_op("trim", args, |s| s.trim().to_string()) |
There was a problem hiding this comment.
trim previously could avoid constructing an intermediate String by directly converting the trimmed &str into Arc<str>. With unary_text_op requiring Fn(&str) -> String, trim() now always allocates a String, which is avoidable for slice-based transforms. Consider adjusting unary_text_op to accept something like Cow<'_, str> (borrow for trim, own for to_uppercase/etc.), or add a second helper specialized for Fn(&str) -> &str/borrowed results so trim can remain allocation-minimal.
| unary_text_op("trim", args, |s| s.trim().to_string()) | |
| check_arg_count("trim", &args, 1)?; | |
| let text = expect_text(&args[0])?; | |
| let trimmed = text.trim(); | |
| Ok(Value::Text(Arc::from(trimmed))) |
| check_arg_count(func_name, &args, 1)?; | ||
| let text = expect_text(&args[0])?; | ||
| Ok(Value::Text(Arc::from(op(&text)))) |
There was a problem hiding this comment.
These calls rely on deref coercions from &Arc<str> to &str (op(&text) / op(&a, &b)), which is valid but a bit opaque. For readability and to make the intended types explicit, prefer passing text.as_ref() (and a.as_ref(), b.as_ref()) into the closures.
| check_arg_count(func_name, &args, 2)?; | ||
| let a = expect_text(&args[0])?; | ||
| let b = expect_text(&args[1])?; | ||
| Ok(Value::Bool(op(&a, &b))) |
There was a problem hiding this comment.
These calls rely on deref coercions from &Arc<str> to &str (op(&text) / op(&a, &b)), which is valid but a bit opaque. For readability and to make the intended types explicit, prefer passing text.as_ref() (and a.as_ref(), b.as_ref()) into the closures.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/stdlib/text.rs (1)
131-133: Nit:trimincurs an extra allocation via.to_string().
s.trim()returns a&str(a borrowed slice of the original), so.to_string()copies the data into a newStringeven when there's no leading/trailing whitespace. This is the conscious trade-off noted in the PR description and is fine for maintainability — just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/text.rs` around lines 131 - 133, The current native_trim uses s.trim().to_string(), which always allocates; change native_trim (and the closure passed to unary_text_op) to check whether s.trim() is equal to s and only allocate when different—return the original owned string/Value when unchanged to avoid an unnecessary allocation, otherwise return s.trim().to_string(); reference native_trim and the closure passed into unary_text_op to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/stdlib/text.rs`:
- Around line 131-133: The current native_trim uses s.trim().to_string(), which
always allocates; change native_trim (and the closure passed to unary_text_op)
to check whether s.trim() is equal to s and only allocate when different—return
the original owned string/Value when unchanged to avoid an unnecessary
allocation, otherwise return s.trim().to_string(); reference native_trim and the
closure passed into unary_text_op to locate the change.
Refactored
src/stdlib/text.rsto use generic helpers insrc/stdlib/helpers.rs(unary_text_opandbinary_text_predicate) to reduce code duplication and improve maintainability. Verified with full test suite.PR created automatically by Jules for task 10726499022893340770 started by @logbie
Summary by CodeRabbit